In basic auth check for tokens before call UserSignIn #5725
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5725 +/- ##
==========================================
+ Coverage 37.76% 37.77% +0.01%
==========================================
Files 325 325
Lines 47650 47686 +36
==========================================
+ Hits 17994 18014 +20
- Misses 27062 27074 +12
- Partials 2594 2598 +4
Continue to review full report at Codecov.
|
bkcsoft
added
the
lgtm/need 2
There was a problem hiding this comment.
Heya, thanks for your PR.
Changing login code is always a bit worrisome as it's an area fraught with consequences.
Your proposed login scheme is somewhat concerning to me. Is this used elsewhere? (either in Gitea or elsewhere?) I really don't like the idea of overloading the password field in this way, or of putting tokens in to the username. (Too many places will log usernames.)
If we have to overload fields - which I guess we have to because git won't let us add other fields. My suggestion would be to instead overload the username field with the type of login plus a separator not allowed in usernames on Gitea (likely +) followed by the username. The password field could then be kept for secrets.
If I'm being stupid and this is already in Gitea and you're just fixing a bug please tell me and I'll look again.
I do think this is a good idea though.
|
@zeripath, I think there is a bug. #5701 The {USER}:{TOKEN} and {TOKEN}:x-oauth-basic are already using for cloning repos, I'm not adding new authentication schemes. When you use the token to clone a repo, actually gitea check first the user, twice, with UserSignIn. If the user is a gitea user (not an external user) this is not a problem, but if you are validating user to LDAP, for example, you are validating the tokens to LDAP: you can block your user since you are sending the username and the token as the password (not your user passwd) or you can ex-filtrate the token if you use the token as the username and "x-oauth-basic" as the password. For every clone you are doing 4 user validations, it's very easy to block or disable the user in the LDAP. In routers/repo/http.go I only change the order of the validation, first I check if is a token and then the password of the user. In modules/auth/auth.go, SignedInUser is call in the middelware Contexter for every request. I didn't find an easy way to know if we are cloning a repo. There is already a check for the api that is very simple (checks if the path begin with "/api/") but detect if the path is a route for Git smart HTTP will be more complicated. With the Git smart HTTP requests there is a problem of double validation that is not solve in this pull. |
lafriks
added
type/bug
issue/critical
lafriks
removed
the
issue/critical
|
Thanks for PR! This has been troubling me for some time on one instance with LDAP :) |
GiteaBot
added
lgtm/need 1
GiteaBot
added
lgtm/done
|
Make lgtm work |
|
Please backport this to release/v1.7 branch |
* Check first if user/password is a token * In basic auth check if user/password is a token * Remove unnecessary else statement * Changes of fmt
With this change we fix #5701